[ESQL] Support first/last functions. #136419

mouhc1ne · 2025-10-10T16:17:26Z

Change AggregatorImplementor to use Argument instead of AggregationParameter.
Compiling esql module resulted in only 2 modified SpatialExtent* classes

- Change `AggregatorImplementor` to use `Argument` instead of `AggregationParameter`. - Compiling esql module resulted in only 2 modified `SpatialExtent*` classes

elasticsearchmachine · 2025-10-10T19:45:19Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

ivancea

LGTM!

ivancea · 2025-10-14T10:20:49Z

.../esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/argument/StandardArgument.java

+        if (invokeBlockEval.length == 1) {
+            builder.addStatement(invokeBlockEval[0]);
+            builder.endControlFlow();
+        } else {


We can remove this condition here and just do the other one always

I agree. Just have the for loop code, and skip the length==1 check.

ivancea · 2025-10-14T10:21:58Z

.../esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/argument/StandardArgument.java

+        if (type.equals(BYTES_REF)) {
+            params += ", " + name + "Scratch";


We can reuse isBytesRef() and scratchName() here (?)

ivancea · 2025-10-14T10:31:50Z

...ugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/AggregatorImplementer.java

        MethodSpec.Builder builder = initAddRaw(true, masked);
-        if (aggParams.getFirst().isArray()) {
+        if (aggParams.getFirst() instanceof BlockArgument) {
            builder.addComment("This type does not support vectors because all values are multi-valued");


Not related with this PR, but I think this should be an assertion? If for any reason code goes through this, it feels like an error.

However, now that you're not calling it, maybe we shouldn't create this method to begin with, if it won't be usable. Can we skip its creation?

If it adds extra complexity, it can be left for another PR

Btw, I don't know much about why these functions can't receive single-valued positions; @craigtaverner will know better if removing/asserting will change logics in any way

I'm not quite sure I'm following the code here. For the spatial point types, we should get vectors of single valued fields. The new code seems to disable this. I think I would need to investigate further to understand why these changes have this effect, because at first glance the switch from isArray to BlockArgument seems correct, but is clearly not working as I'd expect.

craigtaverner

I did a quick review, and it seems to be like something is not quite right. It looks like the vector optimization for spatial points has been disabled.

craigtaverner · 2025-10-15T09:35:59Z

...k/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/argument/Argument.java

     */
    void read(MethodSpec.Builder builder, boolean blockStyle);

+    void read(MethodSpec.Builder builder, String accessor, String firstParam);


Probably needs javadoc like the adjacent methods.

Also, since all but one implementation is an empty implementation, we could use a default implementation here in the interface?

craigtaverner · 2025-10-15T09:40:53Z

...gin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/argument/ArrayArgument.java

        builder.beginControlFlow("for (int i = 0; i < $LBlocks.length; i++)", name);
        builder.addStatement("$LVectors[i] = $LBlocks[i].asVector()", name, name);
-        builder.beginControlFlow("if ($LVectors[i] == null)", name).addStatement(invokeBlockEval).endControlFlow();
+        builder.beginControlFlow("if ($LVectors[i] == null)", name).addStatement(invokeBlockEval[0]).endControlFlow();


Perhaps we should assert that the array has exactly one element?

craigtaverner · 2025-10-15T09:41:22Z

...gin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/argument/ArrayArgument.java

    }

+    @Override
+    public void read(MethodSpec.Builder builder, String accessor, String firstParam) {


This could be deleted if we make a default method in the interface.

craigtaverner · 2025-10-15T09:43:08Z

.../esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/argument/StandardArgument.java

+        if (invokeBlockEval.length == 1) {
+            builder.addStatement(invokeBlockEval[0]);
+            builder.endControlFlow();
+        } else {


I agree. Just have the for loop code, and skip the length==1 check.

craigtaverner · 2025-10-15T10:07:59Z

...ugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/AggregatorImplementer.java

-            builder.addStatement("$T $L = $L.asVector()", vectorType(p.type()), p.vectorName(), p.blockName());
-            builder.beginControlFlow("if ($L == null)", p.vectorName());
+
+        boolean isBlockArgument = aggParams.getFirst() instanceof BlockArgument;


If I understand correctly, we're making the decision if the field is stored as a vector rather than a block up-front in the code generation, instead of at runtime in the evaluator? The consequences for the spatial extents function are that vector optimizations will never occur, and the assumption here is that they never can occur anyway. I'm not sure I quite follow that assumption.

If I remember correctly, we use vector optimization if all the values in the segment are single-valued. And the evaluators that were changed here were spatial extent aggregation over geo_point and cartesian point, which are almost always single-valued in the data-sets I've worked with.

craigtaverner · 2025-10-15T10:44:53Z

...arch/compute/aggregation/spatial/SpatialExtentCartesianShapeDocValuesAggregatorFunction.java

-      return;
-    }
-    addRawVector(valuesVector, mask);
+    addRawBlock(valuesBlock, mask);


Feels like we've disabled the optimization for single-valued points. I'd expect a possible performance regression here.

craigtaverner · 2025-10-15T10:47:07Z

...ugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/AggregatorImplementer.java

        MethodSpec.Builder builder = initAddRaw(true, masked);
-        if (aggParams.getFirst().isArray()) {
+        if (aggParams.getFirst() instanceof BlockArgument) {
            builder.addComment("This type does not support vectors because all values are multi-valued");


I'm not quite sure I'm following the code here. For the spatial point types, we should get vectors of single valued fields. The new code seems to disable this. I think I would need to investigate further to understand why these changes have this effect, because at first glance the switch from isArray to BlockArgument seems correct, but is clearly not working as I'd expect.

[ESQL] Support first/last functions.

d5c6058

- Change `AggregatorImplementor` to use `Argument` instead of `AggregationParameter`. - Compiling esql module resulted in only 2 modified `SpatialExtent*` classes

mouhc1ne self-assigned this Oct 10, 2025

elasticsearchmachine added the v9.3.0 label Oct 10, 2025

mouhc1ne marked this pull request as ready for review October 10, 2025 19:16

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Oct 10, 2025

mouhc1ne added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL >enhancement labels Oct 10, 2025

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Oct 10, 2025

ivancea self-requested a review October 14, 2025 10:32

ivancea approved these changes Oct 14, 2025

View reviewed changes

craigtaverner requested changes Oct 15, 2025

View reviewed changes

Merge branch 'main' into agg-implementer-changes

0f151af

		if (type.equals(BYTES_REF)) {
		params += ", " + name + "Scratch";

[ESQL] Support first/last functions. #136419

Are you sure you want to change the base?

[ESQL] Support first/last functions. #136419

Conversation

mouhc1ne commented Oct 10, 2025

Uh oh!

elasticsearchmachine commented Oct 10, 2025

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigtaverner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigtaverner Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

craigtaverner Oct 15, 2025 •

edited

Loading